Skip to content

feat(mcp-server): implement hyperping[mcp-server] SDK#43

Draft
KhaledSalhab-Develeap wants to merge 3 commits into
mainfrom
feat/5bf946-mcp-server
Draft

feat(mcp-server): implement hyperping[mcp-server] SDK#43
KhaledSalhab-Develeap wants to merge 3 commits into
mainfrom
feat/5bf946-mcp-server

Conversation

@KhaledSalhab-Develeap

Copy link
Copy Markdown
Collaborator

@PR_BODY.md

Adds [mcp-server] optional dependency group and mcp to [dev] deps
so tests can import it. Lock file regenerated.
Tests covering factory behaviour, per-group tool counts, delegation
to mock clients, destructive-operation return shapes, and CLI
argument parsing. 43 tests total across 9 test modules.
Exposes all 62 Hyperping operations as agent-callable MCP tools via a
create_mcp_server() factory function backed by FastMCP.

- 7 domain modules: monitors (10), incidents (7), maintenance (7),
  outages (8), statuspages (8), healthchecks (7), observability (15)
- Factory accepts api_key, pre-configured HyperpingClient, or
  HyperpingMcpClient; tools= kwarg filters to a subset of groups
- CLI entrypoint: python -m hyperping.mcp_server --transport stdio|sse
- Lazy re-export of create_mcp_server from hyperping.__init__ keeps
  mcp import optional at base package load time
@KhaledSalhab-Develeap

Copy link
Copy Markdown
Collaborator Author

Self-review — 5-lens pass

All 43 unit tests pass. Full unit suite (643 tests) passes with no regressions.


Findings

Medium — __main__.py:35-38,75--port is dead code

_build_parser registers --port (default 8080, described as "Port for the SSE transport"), and the module docstring example shows --transport sse --port 8080. But server.run(transport=args.transport) at line 75 never receives the port, and create_mcp_server() has no port parameter to forward it to FastMCP().

FastMCP.__init__ accepts port: int = 8000 as a keyword argument; that is the correct insertion point. Without the fix, any SSE deployment gets FastMCP's built-in default (8000) regardless of --port. The CLI advertises a control that silently does nothing.

Fix: Add port: int = 8000 to create_mcp_server(), pass it to FastMCP(name, port=port), and forward args.port in main().


Low — _tools_monitors.py:161-165search_monitors_by_name raises at call time when mcp_client is None; not tested

The observability group is not registered at all when mcp_client is None (_registry.py:91). search_monitors_by_name takes the opposite approach: it is always registered but raises RuntimeError at call time if mcp_client is None. A user who calls create_mcp_server(client=c, tools=["monitors"]) (no MCP client) will discover the failure only at invocation, not at tool listing. The RuntimeError path has no test.

Fix (optional, acceptable as followup): either guard registration the same way the observability group does, or add a test that asserts the RuntimeError is raised with the expected message.


Nit — __main__.py:30"streamable-http" missing from --transport choices

FastMCP.run() accepts 'stdio' | 'sse' | 'streamable-http', but the CLI only allows ["stdio", "sse"]. A user who wants streamable-HTTP must bypass the CLI.


Nit — test_factory.py:51-58 — duplicate test

test_create_server_with_mcp_client is identical to test_create_server_with_client (same fixtures, same assertion). Adds no coverage.


Refuted findings

  • Boolean filter (if v is not None): Initially flagged as dropping follow_redirects=False. False is not None evaluates True, so False passes the filter. Not a bug.
  • from hyperping import * with optional mcp: __getattr__("create_mcp_server") imports hyperping.mcp_server, which does not import from mcp at module level (TYPE_CHECKING guard). Star import is safe.
  • Thread safety: httpx sync Client is thread-safe; shared client instance across concurrent tool calls is fine.
  • Tool count headline: Ticket says 66, implementation delivers 62. The ticket's own breakdown (46 REST + 15 MCP-client + 1 search = 62) is internally consistent; "66" is a typo in the headline. No code change needed.

Verdict

PASS. One medium finding (--port dead code) and two nits. No blockers or high findings. The medium finding is a functional gap for SSE deployments but does not affect the primary stdio and in-process use cases.

Followups not in this PR

  1. Wire --port through create_mcp_server(port=) to FastMCP(name, port=port) so SSE deployments can control the bind port via CLI.
  2. Add a test for search_monitors_by_name with mcp_client=None to pin the RuntimeError path.
  3. Add "streamable-http" to --transport choices in the CLI.
  4. Remove or repurpose the duplicate test_create_server_with_mcp_client test.

@ksalhab89

Copy link
Copy Markdown

Reviewer context (not a merge request):

Implements the hyperping[mcp-server] extra: a create_mcp_server factory plus a python -m hyperping.mcp_server CLI, registering tool groups (monitors, incidents, maintenance, outages, statuspages, healthchecks, observability).

Where to focus review: mcp_server/__init__.py create_mcp_server (client/mcp_client construction precedence, ValueError when neither api_key nor client given) and _registry.py register_tools (group validation via _VALID_GROUPS, observability silently skipped when mcp_client is None). Check the lazy __getattr__ hook in top-level __init__.py that imports create_mcp_server only on access to avoid importing optional mcp at import time. Confirm the ImportError message points to the right extra.

Risks / verify: Tool docstrings are model-facing; ensure accuracy. Confirm the --transport sse path in __main__.py works with FastMCP. Additive only, no changes to existing public API.

CI status: Green. CI ran on this branch: test (3.11/3.12/3.13) all SUCCESS.

Notes: Base of the stack for #46. Looks safe; review this first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants